Skip to content

Prefer map syntax for services#5988

Draft
6543 wants to merge 14 commits intowoodpecker-ci:mainfrom
6543-forks:types.services.serialize.as.map
Draft

Prefer map syntax for services#5988
6543 wants to merge 14 commits intowoodpecker-ci:mainfrom
6543-forks:types.services.serialize.as.map

Conversation

@6543
Copy link
Member

@6543 6543 commented Jan 18, 2026

as services must have unique names as they are used for there host names, and services start all in parallel anyway, we should prefer map syntax.

because the map syntax in yaml represents how they work best:

woodpecker/.woodpecker/securityscan.yaml

Lines 34 to 41 in 4e0b91f

services:
server:
image: *trivy_plugin
settings:
service: true
db-repository: mirror.gcr.io/aquasec/trivy-db:2
ports:
- 10000

woodpecker/.woodpecker/test.yaml

Lines 144 to 159 in 4e0b91f

services:
postgres:
image: docker.io/postgres:18
ports: ['5432']
environment:
POSTGRES_USER: postgres
POSTGRES_HOST_AUTH_METHOD: trust
when: *when
mysql:
image: docker.io/mysql:9.5.0
ports: ['3306']
environment:
MYSQL_DATABASE: test
MYSQL_ALLOW_EMPTY_PASSWORD: 'yes'
when: *when

this change also makes sure re-serialize works just fine :)

@6543 6543 added documentation docu & docs enhancement improve existing features pipeline-config related to pipeline config files (.yaml) labels Jan 18, 2026
@woodpecker-bot
Copy link
Contributor

woodpecker-bot commented Jan 18, 2026 •
edited
Loading

Surge PR preview deployment succeeded. View it at https://woodpecker-ci-woodpecker-pr-5988.surge.sh

@6543
Copy link
Member Author

6543 commented Jan 18, 2026 •
edited
Loading

this pr also to be used in https://codeberg.org/lafriks/woodpecker-pipeline-transform/pulls/33#issuecomment-9962346

6543
6543 commented Jan 18, 2026
View reviewed changes
@qwerty287
Copy link
Contributor

qwerty287 commented Jan 19, 2026

I don't like this change. It would be nice if steps and services have a consistent syntax.

I personally think we should just forbid duplicated names (e.g. via the linter), for both steps and services.

6543 added a commit to 6543-forks/woodpecker that referenced this pull request Jan 19, 2026
@6543
Add ReSerialize test from woodpecker-ci#5988
2b00116
@6543 6543 mentioned this pull request Jan 19, 2026
Merged
@6543

This comment was marked as resolved.

Sign in to view
@lafriks
Copy link
Contributor

lafriks commented Jan 19, 2026

I don't like this change. It would be nice if steps and services have a consistent syntax.

I personally think we should just forbid duplicated names (e.g. via the linter), for both steps and services.

I agree, the linter should check for duplicate names, I also prefer list not a map syntax

@6543
Copy link
Member Author

6543 commented Jan 21, 2026

I removed the warning and added an check if it is a list and was declared twice

@6543
Copy link
Member Author

6543 commented Jan 21, 2026

also we support duplicated step names and it has no conflict so why should we now block it?

@codecov

This comment was marked as resolved.

Sign in to view
@qwerty287
Copy link
Contributor

qwerty287 commented Jan 21, 2026

Because it's not working properly: #4385

(Didn't try myself, but since that issue was opened nothing was changed to fix it)

@6543
Copy link
Member Author

6543 commented Jan 21, 2026

might have missed that issue but it should be solved since worker api for steps changed to uuid for identifyers

@qwerty287
Copy link
Contributor

qwerty287 commented Jan 22, 2026

I think this issue was opened after we changed that. But I would also have to try that first.

@qwerty287
Copy link
Contributor

qwerty287 commented Jan 22, 2026

I just tested it again, and the issue is the depends_on. See https://ci.codeberg.org/repos/16087/pipeline/3.

The test-1 pipeline has a depends_on to the duplicated step build. And then only one build step runs. If you remove the depends_on it works (test-2 pipeline).

@6543
Copy link
Member Author

6543 commented Jan 22, 2026 •
edited
Loading

well depends_on is a fundamental feature ... forgot about that conflict ... so in this case as soon as it is used we need to lint for that.

i have a look what is more complex (denai of it allways or just at the dag case)

qwerty287
qwerty287 reviewed Jan 29, 2026
View reviewed changes
pipeline/frontend/yaml/types/container_map.go
// ContainerMap contains collection of containers.
type ContainerMap struct {
ContainerMap map[string]*Container
Duplicated []string
Copy link
Contributor

@qwerty287 qwerty287 Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to put this into the parsing code? I think the linter should check separately for duplicated entries and for parsing just use the ContainerList

@qwerty287 qwerty287 marked this pull request as draft March 6, 2026 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qwerty287 qwerty287 qwerty287 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

documentation docu & docs enhancement improve existing features pipeline-config related to pipeline config files (.yaml)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@6543 @woodpecker-bot @qwerty287 @lafriks